Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(item-divider): add inner padding CSS variables #18490

Merged
merged 9 commits into from Jun 10, 2019
Merged

feat(item-divider): add inner padding CSS variables #18490

merged 9 commits into from Jun 10, 2019

Conversation

corollari
Copy link
Contributor

@corollari corollari commented Jun 8, 2019 •
edited by brandyscarney

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: fixes #18484

What is the new behavior?

It solves: #18484

Does this introduce a breaking change?

  • Yes
  • No

Other information

All credit goes to @brandyscarney, I just implemented the solution she proposed on #18484

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Jun 8, 2019
@liamdebeasi
Copy link
Contributor

Hey there,

Just wanted to mention a couple things before we merge:

  1. Since these are new CSS variables, we need to document them (This will make this PR a feature instead of a bug fix).
  2. The variables need to be initialized before we can use them. You can just initialize them to 0.

Can you make these changes? I can take another look after. Thanks!

@corollari
Copy link
Contributor Author

corollari commented Jun 10, 2019 •
edited

Edit: Nevermind, just solved my question.

@corollari
Copy link
Contributor Author

@liamdebeasi Just applied the changes you requested. Not sure if I should document the variables in a different way, right now it feels very copy-pasty.

@brandyscarney brandyscarney changed the title Fix #18484 feat(item-divider): add inner padding CSS variables Jun 10, 2019
@brandyscarney
Copy link
Member

Thank you @corollari! I added a few small changes. I'll let Liam take another look, but looks good to me! 🙂

@brandyscarney brandyscarney self-requested a review June 10, 2019 19:03
@corollari
Copy link
Contributor Author

@brandyscarney cool, it seems I forgot to build the documentation.
Also, given that this is now a new feature, should I change the commit messages? or is that not needed because of the pre-merge squash?

@brandyscarney
Copy link
Member

@corollari It's not needed, we'll update it on squash & merge. Thank you though!

@brandyscarney brandyscarney merged commit 35c143a into ionic-team:master Jun 10, 2019
@brandyscarney
Copy link
Member

Thanks again! 🙂

@corollari
Copy link
Contributor Author

Kudos for the awesome handling of this PR 😄

abennouna pushed a commit to abennouna/ionic that referenced this pull request Jun 11, 2019
@corollari @abennouna
feat(item-divider): add inner padding CSS variables (ionic-team#18490)
f570dd0 
fixes ionic-team#18484
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liamdebeasi liamdebeasi liamdebeasi approved these changes

@brandyscarney brandyscarney brandyscarney approved these changes

Assignees
No one assigned
Labels
package: core @ionic/core package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: ion-item-divider (iOS) has no "--padding-inline-end" variable
3 participants
@corollari @liamdebeasi @brandyscarney